Skip to content

MDG Public API python wrapper #125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
william-lukusa merged 4 commits into from
May 31, 2021
Merged

MDG Public API python wrapper #125

william-lukusa merged 4 commits into from
May 31, 2021

Conversation

@william-lukusa
Copy link
Contributor

@william-lukusa william-lukusa commented Mar 8, 2021

Story details : [ch58204]

⚠️ use and merge with https://github.com/dataiku/dip/pull/12168

@william-lukusa william-lukusa self-assigned this Mar 8, 2021
@shortcut-integration
Copy link

shortcut-integration bot commented Mar 8, 2021

This pull request has been linked to Clubhouse Story #58204: MDG: Public API.

FlorentDev64
FlorentDev64 requested changes Apr 16, 2021
View reviewed changes
FlorentDev64
FlorentDev64 approved these changes Apr 16, 2021
View reviewed changes
Copy link
Contributor

@FlorentDev64 FlorentDev64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

FlorentDev64
FlorentDev64 reviewed Apr 19, 2021
View reviewed changes
Copy link
Contributor

@FlorentDev64 FlorentDev64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this two routes should be merged into one single route to avoid code duplication.
"GET", "/projects/%s/models/lab/documentations/%s" % (self.mltask.project_key, export_id))
"GET", "/projects/%s/savedmodels/documentations/%s" % (self.saved_model.project_key, export_id))

Because, we do not need to know where the documentation came from (savedModels or 'models/labs). Maybe we should have one single route like "GET", "/projects/%sdocumentations/%s" % (self.saved_model.project_key, export_id))`

If this is done, we should move the route to another file.

@william-lukusa
Copy link
Contributor Author

william-lukusa commented Apr 19, 2021

Because, we do not need to know where the documentation came from (savedModels or 'models/labs). Maybe we should have one single route like "GET", "/projects/%sdocumentations/%s" % (self.saved_model.project_key, export_id))`

@pbailly @adescamps what do you think ? We discussed it and I still think having the route /documentations/{exportId} in projects might be confusing and keeping it like that is better in the documentation. But I'd like to have some more feedback 😄

@pbailly pbailly added this to the 9.0.3 milestone Apr 19, 2021
@pbailly pbailly modified the milestone: 9.0.3 May 10, 2021
@william-lukusa william-lukusa merged commit bc2828e into release/9.0 May 31, 2021
@pbailly pbailly deleted the feature/dss902-mdg-public-api-wrapper branch June 2, 2021 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FlorentDev64 FlorentDev64 FlorentDev64 approved these changes

@pbailly pbailly Awaiting requested review from pbailly

@apichery apichery Awaiting requested review from apichery

Assignees

@william-lukusa william-lukusa

Labels

None yet

Projects

None yet

Milestone

9.0.3

Development

Successfully merging this pull request may close these issues.

4 participants

@william-lukusa @FlorentDev64 @pbailly